feat(useSetting): sync values with LS and store in api #3111

artemmufazalov · 2025-11-19T17:35:27Z

Part of #2892

Currently it can be checked with table columns width.
Stand (without meta): https://nda.ya.ru/t/BEIjuuLI7N8Qd7
Stand (with meta): https://nda.ya.ru/t/Y60-A5AI7NF3Mi

Sync all values within api onQueryStarted instead of useEffects.

This approach proved itself good with and without meta api. Without meta api there is only 1 failed test (against 100-200 before): #3054

greptile-review

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total	Passed	Failed	Flaky	Skipped
378	375	0	1	2

Test Changes Summary ⏭️2

⏭️ Skipped Tests (2)

Scroll to row, get shareable link, navigate to URL and verify row is scrolled into view (tenant/diagnostics/tabs/queries.test.ts)
Copy result button copies to clipboard (tenant/queryEditor/queryEditor.test.ts)

Bundle Size: ✅

Current: 66.13 MB | Main: 66.13 MB
Diff: +2.99 KB (0.00%)

✅ Bundle size unchanged.

ℹ️ CI Information

Test recordings for failed tests are available in the full report.
Bundle size is measured for the entire 'dist' directory.
📊 indicates links to detailed reports.
🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Copilot

Pull Request Overview

This PR refactors the settings synchronization architecture by moving the sync logic from useEffect hooks in useSetting.ts to RTK Query's onQueryStarted lifecycle hooks in the API layer. The main goal is to centralize settings synchronization between localStorage, Redux store, and the meta settings API.

Key Changes:

Consolidates all settings sync logic into the settingsApi endpoints' onQueryStarted hooks
Simplifies useSetting hook by removing multiple useEffect dependencies
Handles localStorage and API settings synchronization at the API layer instead of component layer

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`src/store/reducers/settings/utils.ts`	Adds helper functions `getSettingDefault` and `shouldSyncSettingToLS` to support centralized settings sync logic
`src/store/reducers/settings/useSetting.ts`	Removes all `useEffect`-based sync logic and simplifies hook to only trigger API queries and mutations
`src/store/reducers/settings/constants.ts`	Updates `SETTINGS_OPTIONS` type to explicitly allow `undefined` values for better type safety
`src/store/reducers/settings/api.ts`	Implements comprehensive sync logic in `onQueryStarted` hooks for both query and mutation endpoints, handling localStorage and store updates
`src/services/api/metaSettings.ts`	Updates return type to allow `undefined` for settings that don't exist in the backend

src/store/reducers/settings/useSetting.ts

src/store/reducers/settings/api.ts

src/store/reducers/settings/utils.ts

src/store/reducers/settings/api.ts

Raubzeug · 2025-11-20T06:39:36Z

@greptile-review

greptile-apps

_{8 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/store/reducers/settings/api.ts

astandrik · 2025-11-20T13:23:57Z

src/store/reducers/settings/api.ts


                    return {data};
                } catch (error) {
+                    console.error('Cannot get settings:', error);


Can it be that error is "Cannot get setting" - but the real situation is that one of patches failed ?

astandrik · 2025-11-20T13:27:31Z

Now as I see we have three sources of truth:

LS
Backend
Redux Store

And we struggle to keep all of them in sync
Is my assumption correct?

artemmufazalov · 2025-11-20T13:38:10Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

astandrik · 2025-11-20T14:06:23Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

Is it a goal or temporary state?

artemmufazalov · 2025-11-20T14:16:44Z

Is it a goal or temporary state?

It's a goal

astandrik · 2025-11-20T14:28:49Z

src/store/reducers/settings/api.ts

+                        const newValue = data ?? {name, user};
+
+                        // Try to sync local value if there is no backend value
+                        syncLocalValueToMetaIfNoData(newValue, dispatch);


is it possible that null/undefined on backend is intentional?
i mean we removed some setting for example

but LS has some old value (in another browser for example) and pushes this setting to backend?

It's a current limitation, yes. We cannot have null or undefined as backend value, there should be at least something, otherwise defined localStorage value will be loaded to backend. Without this it's unclear, when we should sync local value to backend

astandrik · 2025-11-20T14:32:59Z

src/store/reducers/settings/api.ts

+        return;
+    }
+
    const localValue = localStorage.getItem(params.name);


we use localStorage directly here but there are already functions for this like

readSettingValueFromLS / shouldSyncSettingToLS

these functions as I may suppose were created to make work with LS somehow guarded

astandrik · 2025-11-20T14:37:55Z

Now as I see we have three sources of truth:

LS

Backend

Redux Store

And we struggle to keep all of them in sync Is my assumption correct?

Yes, everything is to keep them in sync

I thought this over again and came to a conclusion that actually there is one source of truth (backend) while LS and rtq storage are used as caches for interface rensponsiveness so actually its not "three sources of truth" but "one with two caches"

Does it make sense?

artemmufazalov · 2025-11-20T14:58:26Z

I thought this over again and came to a conclusion that actually there is one source of truth (backend) while LS and rtq storage are used as caches for interface rensponsiveness so actually its not "three sources of truth" but "one with two caches"

Does it make sense?

In general, you are right. But it depends on app installation. There are 3 cases.

No meta (embedded single cluster version). LS is used as source of truth, meta is not used at all, we read and load value to redux to use it in the app without a need to read it from LS.
There is meta service with settings. In this case value from LS is used as temporary initial value while setting from meta is not loaded. When value is loaded, it is used.

So, preload data from LS to store, load actual value from meta when request is finished.

There is meta service, but there is no meta setting value. It is possible case in migration period - users have all their setting in LS now. It is similar to case 2, but when we receive undefined value from backend, we load value from LS there

In all cases redux is used as sync cache for data.

astandrik · 2025-11-20T15:08:47Z

So there are two main scenarios with mainly opposite mechanics

with meta
without meta

It appears we try to handle both at once with one handler and therefore it looks a bit complex

I'm trying to imagine if we could explicitly split these scenarios for clarity and maintainability

its possible to postpone this and separate to some tech issue but would be great if we could handle it at once

feat(useSetting); sync values with LS and store in api

7f5c82e

artemmufazalov changed the title ~~feat(useSetting); sync values with LS and store in api~~ feat(useSetting): sync values with LS and store in api Nov 19, 2025

artemmufazalov requested a review from Copilot November 19, 2025 17:36

artemmufazalov mentioned this pull request Nov 19, 2025

Support YDB settings service in EM #2892

Open

Copilot started reviewing on behalf of artemmufazalov November 19, 2025 17:36 View session

Copilot finished reviewing on behalf of artemmufazalov November 19, 2025 17:39

Copilot AI reviewed Nov 19, 2025

View reviewed changes

artemmufazalov added 2 commits November 19, 2025 22:06

fix: copilot review

cbfadb0

fix: failed test

b5eeee2

artemmufazalov marked this pull request as ready for review November 19, 2025 20:11

artemmufazalov requested review from Raubzeug, adameat and astandrik as code owners November 19, 2025 20:11

greptile-apps bot reviewed Nov 20, 2025

View reviewed changes

src/store/reducers/settings/api.ts Outdated Show resolved Hide resolved

src/store/reducers/settings/api.ts Show resolved Hide resolved

fix: log errors

29a9e3d

astandrik reviewed Nov 20, 2025

View reviewed changes

feat(useSetting): sync values with LS and store in api #3111

Are you sure you want to change the base?

feat(useSetting): sync values with LS and store in api #3111

Uh oh!

Conversation

artemmufazalov commented Nov 19, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Results

Test Status: ⚠️ FLAKY

⏭️ Skipped Tests (2)

Bundle Size: ✅

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Raubzeug commented Nov 20, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

astandrik Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

astandrik commented Nov 20, 2025

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

astandrik Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

artemmufazalov Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

astandrik Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemmufazalov commented Nov 20, 2025

Uh oh!

astandrik commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

artemmufazalov commented Nov 19, 2025 •

edited by github-actions bot

Loading

astandrik commented Nov 20, 2025 •

edited

Loading

astandrik commented Nov 20, 2025 •

edited

Loading

astandrik commented Nov 20, 2025 •

edited

Loading